Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nudge kernel with info request until we receive IOPub messages #361

Merged
merged 2 commits into from
Dec 21, 2020

Conversation

SylvainCorlay
Copy link
Contributor

@SylvainCorlay SylvainCorlay commented Dec 11, 2020

Porting jupyter/notebook#5908 to jupyter_server.

see original discussion in jupyter/jupyter_client#593

Nudge kernel with info request upon opening websocket and upon restart until some IOPub message is received.

@codecov-io
Copy link

codecov-io commented Dec 12, 2020

Codecov Report

Merging #361 (39eefe7) into master (6fef2a8) will decrease coverage by 1.26%.
The diff coverage is 18.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
- Coverage   68.24%   66.98%   -1.27%     
==========================================
  Files          56       56              
  Lines        6066     6134      +68     
  Branches      794      804      +10     
==========================================
- Hits         4140     4109      -31     
- Misses       1693     1784      +91     
- Partials      233      241       +8     
Impacted Files Coverage Δ
jupyter_server/services/kernels/handlers.py 45.74% <18.42%> (-6.52%) ⬇️
jupyter_server/__init__.py 61.53% <0.00%> (-38.47%) ⬇️
jupyter_server/terminal/api_handlers.py 44.11% <0.00%> (-29.42%) ⬇️
jupyter_server/terminal/handlers.py 57.89% <0.00%> (-26.32%) ⬇️
jupyter_server/terminal/__init__.py 76.00% <0.00%> (-16.00%) ⬇️
jupyter_server/_sysinfo.py 81.81% <0.00%> (-12.13%) ⬇️
jupyter_server/utils.py 58.79% <0.00%> (-12.07%) ⬇️
jupyter_server/services/contents/fileio.py 72.25% <0.00%> (-2.10%) ⬇️
jupyter_server/services/contents/filemanager.py 65.53% <0.00%> (-1.71%) ⬇️
jupyter_server/services/kernels/kernelmanager.py 61.90% <0.00%> (-0.80%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fef2a8...39eefe7. Read the comment docs.

@SylvainCorlay
Copy link
Contributor Author

Now that jupyter/notebook#5908 has been merged, I think that this can get in!

cc @minrk

Copy link
Member

@Zsailer Zsailer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @SylvainCorlay. Took for a spin (with minrk/ipykernel@3fdb063 from @minrk) and it works beautifully!

@SylvainCorlay
Copy link
Contributor Author

I think this is ready. Please feel free to merge! This issue has been bothering us for quite some time!

@Zsailer Zsailer merged commit dd567f6 into jupyter-server:master Dec 21, 2020
@Zsailer
Copy link
Member

Zsailer commented Dec 21, 2020

Done and released! jupyter_server 1.1.2

@kevin-bates
Copy link
Member

kevin-bates commented Apr 8, 2021

Hi @SylvainCorlay - I'm sorry for posting this against a closed PR, but I figured it's got the most context relative to what I'm seeing across restarts when kernels have had their ports changed.

I'd like to get a better understanding of these lines of code in nudge():

# Use a transient shell channel to prevent leaking
# shell responses to the front-end.
shell_channel = kernel.connect_shell()
# The IOPub used by the client, whose subscriptions we are verifying.
iopub_channel = self.channels["iopub"]

which are really artifacts (for the purpose of this discussion) of these lines of code:
# on new connections, flush the message buffer
buffer_info = km.get_buffer(kernel_id, self.session_key)
if buffer_info and buffer_info['session_key'] == self.session_key:
self.log.info("Restoring connection for %s", self.session_key)
self.channels = buffer_info['channels']

  1. Do you know why kernel.shell_port is considered transient? The shell_port/channel on the kernel manager is tied to the current shell port so I'm not following what is transient about it.
  2. Since nudge() needs to monitor shell and iopub, and self.channels['iopub'] reflects the iopub_port of the previous kernel manager instance (prior to restart), then when it detects that messages have been buffered, nudge() eventually times out in cases where ports change on a restart. It seems like kernel.iopub_channel is the correct port to tie nudge() to regardless of whether messages have been buffered or not because it will always be the current iopub port/channel.

I also find that the connection counts (via self._kernel_connections) are one higher in Lab than Notebook. I suspect this is because Lab uses a WS in its main file-browser pane - so closing the tab of the notebook window only decrements the connections to 1 - so buffering doesn't take place (it only kicks in when connections are zero). Whereas Notebook doesn't open a WS in its primary window, therefore, simply closing the tab will decrement connections to zero and buffering will start. This difference manifests the nudge timeout issue more easily when restarts occur within Notebook (and ports have changed) because the shutdown-reply sequence will get buffered due to the connection count going to zero when the kernel is shutdown (prior to its (re)start).

I know this is all intricately tied to the buffer replay stuff, but I think self.channels['iopub'] should be changed kernel.connect_iopub() and we should be unconditionally calling self.create_stream() - like we do when buffered messages are not detected.
(edit: calling self.create_steam() will reset self.channels which are necessary for replay to deserialize the buffered messages. I think the issue with buffer replay is its too closely wrapped in the handler instance (self).)

Does this make sense?

Zsailer added a commit to Zsailer/jupyter_server that referenced this pull request Nov 18, 2022
hMED22 pushed a commit to hMED22/jupyter_server that referenced this pull request Jan 23, 2023
Nudge kernel with info request until we receive IOPub messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants